Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QAM public cloud enablement #8422

Merged
merged 1 commit into from Oct 14, 2019
Merged

QAM public cloud enablement #8422

merged 1 commit into from Oct 14, 2019

Conversation

pdostal
Copy link
Member

@pdostal pdostal commented Sep 11, 2019

@pdostal pdostal force-pushed the qampc1 branch 2 times, most recently from d3e1500 to 522b04c Compare September 11, 2019 15:02
@pdostal
Copy link
Member Author

pdostal commented Sep 11, 2019

Implemented pdostal#1 by @cfconrad

@pdostal pdostal force-pushed the qampc1 branch 2 times, most recently from be4ad66 to 71a0594 Compare September 12, 2019 07:14
Copy link
Contributor

@pevik pevik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'd just describe a bit goals of QAM tests (I guess it's the concentration on user space; I mean there is lot's of files named qam, but no explanation in the commit message).

@asmorodskyi
Copy link
Member

LGTM. I'd just describe a bit goals of QAM tests (I guess it's the concentration on user space; I mean there is lot's of files named qam, but no explanation in the commit message).

+1 to this . there is no such thing us "QAM test" , there is just a test which would be executed by someone . Test should say what it actually tests and not who is executing it

@pdostal
Copy link
Member Author

pdostal commented Sep 12, 2019

LGTM. I'd just describe a bit goals of QAM tests (I guess it's the concentration on user space; I mean there is lot's of files named qam, but no explanation in the commit message).

+1 to this . there is no such thing us "QAM test" , there is just a test which would be executed by someone . Test should say what it actually tests and not who is executing it

I agree as well 😄 I will rename it properly.

The purpose of this is to allow us to run existing tests on public cloud.

  1. Currently I use user-console create instance and open SSH tunnel for /dev/sshserial.
  2. Then I switch to root-console and I open interactive SSH session there.
  3. This 'hack' allows me to include existing tests so they continue in the SSH session from 2)
  4. At the end I just destroy the public cloud VM

My idea is to move the logic from user-console to new-separate console, but I don't know how...

@cfconrad
Copy link
Contributor

My idea is to move the logic from user-console to new-separate console, but I don't know how...

I had a conversation with @okurz about such solution in the past. And if I'm not wrong he worked on something similar. Can you comment plz?

@pdostal
Copy link
Member Author

pdostal commented Sep 12, 2019

LGTM. I'd just describe a bit goals of QAM tests (I guess it's the concentration on user space; I mean there is lot's of files named qam, but no explanation in the commit message).

I renamed it to ssh_init and ssh_end perhaps it opens the SSH interactive session.
I kept the qam_test because it has no future and is less selfish than pdostal_test 😄

@pdostal pdostal force-pushed the qampc1 branch 3 times, most recently from 1360dd9 to a5121e2 Compare September 12, 2019 11:38
Copy link
Contributor

@cfconrad cfconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice.
The VR didn't point to explicit job, can you point to one using the latest code?

tests/publiccloud/ssh_init.pm Outdated Show resolved Hide resolved
tests/publiccloud/ssh_init.pm Outdated Show resolved Hide resolved
tests/publiccloud/qam_test.pm Outdated Show resolved Hide resolved
tests/publiccloud/qam_test.pm Outdated Show resolved Hide resolved
tests/publiccloud/qam_test.pm Outdated Show resolved Hide resolved
tests/publiccloud/ssh_end.pm Outdated Show resolved Hide resolved
tests/publiccloud/ssh_end.pm Outdated Show resolved Hide resolved
@pdostal
Copy link
Member Author

pdostal commented Sep 16, 2019

@cfconrad as #8439 is already merged I just force-pushed a new code, where I use $instance->run_ssh_command() but please see lib/publiccloud/instance.pm because of I needed to use script_run() instead of script_output() so I added another argument but the default behavior is still the same.

lib/main_common.pm Outdated Show resolved Hide resolved
lib/main_common.pm Outdated Show resolved Hide resolved
@pdostal pdostal force-pushed the qampc1 branch 2 times, most recently from 28fd7f6 to cdf6356 Compare October 2, 2019 09:11
@pdostal pdostal force-pushed the qampc1 branch 2 times, most recently from e0c5b31 to 110593e Compare October 10, 2019 12:12
Copy link
Contributor

@cfconrad cfconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beside this test_flags() overwrite, everything looks good to me.

lib/consoletest.pm Outdated Show resolved Hide resolved
tests/publiccloud/download_repos.pm Outdated Show resolved Hide resolved
@pdostal pdostal force-pushed the qampc1 branch 3 times, most recently from 0d45e46 to 5b5ebba Compare October 10, 2019 13:18
Copy link
Contributor

@cfconrad cfconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! Let's see how it run :)

@@ -0,0 +1,49 @@
# SUSE's openQA tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cfconrad , I have a feeling that we already have all or most part of this info somewhere ? img_proof tests ? or we gathering this info somewhere ? terraform output ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole test is actually not a test but bunch of commands I used to proof that the SSH console works. I would personally keep it just to see something basic at the beginning but we can remove it as well...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asmorodskyi you are right that img_proof collects some of it and so on. But I actually don't know if QAM team will run img_proof at all. From what I see currently, they are using a different approach to run openQA modules on the instance. I would keep it as well.


assert_script_run("ps aux | nl");

register_product();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might behave differently based on type of image you use , so bad idea use it unconditionally

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you mean that? I just want to install traceroute

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of images available in Public Cloud expecting something like this - they called BYOS - there you getting unregistered image which suppose to register . But others will give you image which already registered via Public Cloud Provider so you don't need to register it and can just use it. Not sure how this type of images will react on attempt to register them via SCC ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I see, well, I would wait with this. Currently I use only one image and nobody decided yet for which images this will be used. The goal for this pull request is to enable the interactive ssh console in the public cloud environment. After that I will be dealing with individual tests and scenarios.

select_console 'root-console';

assert_script_run("rsync -va -e ssh ~/repos root@" . $args->{my_instance}->public_ip . ":'/tmp/repos'", timeout => 900);
$args->{my_instance}->run_ssh_command(cmd => "sudo find /tmp/repos/ -name *.repo -exec sed -i 's,http://,/tmp/repos/repos/,g' '{}' \\;");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you actually connecting as root user , why you do everything via sudo ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't. The default user is ec2-user.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach in general looks ok. I have not found any serious flaws. However there is a bit of duplication related to #8099 . Please take a look into there for my approach, maybe you find it helpful.

@pdostal
Copy link
Member Author

pdostal commented Oct 11, 2019

However there is a bit of duplication related to #8099 . Please take a look into there for my approach, maybe you find it helpful.

Hello @okurz, I tried some of the code from #8099 before, but yesterday, when I saw it all in one PR I managed to actually execute it and implement our use case into it.

Can I ask you to take a look once more?

@pdostal pdostal force-pushed the qampc1 branch 4 times, most recently from 5a6b408 to 341ab8b Compare October 11, 2019 09:28
Copy link
Contributor

@cfconrad cfconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest changes with TUNNELED approach looking good to me!

@okurz
Copy link
Member

okurz commented Oct 11, 2019

yep, I like it as well. Nicely done. Something similar as in https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/lib/Remote/Lab.pm#L66 might be helpful as well but it's not mandatory.

@pdostal
Copy link
Member Author

pdostal commented Oct 12, 2019

@okurz thank you very much.

Something similar as in https://github.com/os-autoinst/os-autoinst-distri-opensuse/blob/master/lib/Remote/Lab.pm#L66 might be helpful as well but it's not mandatory.

There is ssh_interactive_tunnel() in lib/publiccloud/ssh_interactive.pm which I find similar to your setup_ssh_tunnels(). The reason why I didn't reuse it is because I found it too specific for your scenario. For example I don't have any jumphost.

@okurz
Copy link
Member

okurz commented Oct 12, 2019

sure, of course. I did not think it is "reusable", just could provide ideas.

@cfconrad cfconrad merged commit 7c3f7ac into os-autoinst:master Oct 14, 2019
@pdostal pdostal deleted the qampc1 branch December 28, 2022 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants